Review — PR #806 (round 1)

1. Breaking public type change shipped as a minor changeset

packages/harness/src/shared/agent-map.ts:353 replaces the exported ProposalActor
interface with ProjectAgentActorRef, dropping the role and assignment fields:

-export interface ProposalActor { userId; sessionId; role: "map-planner" | "agent-builder"; assignment: … }
+export type ProposalActor = ProjectAgentActorRef;   // { userId; sessionId }

ProposalActor is re-exported from the package root (src/index.ts:38), and it is the
type of ProposalOperationRecord.actor / MapChangeProposal.history[].actor. Any pinned
consumer that reads record.actor.role or record.actor.assignment — or that constructs a
ProposalActor literal — stops compiling on upgrade. The runtime payload of
GET /api/agent-map loses the same two fields.

The changeset (.changeset/neutral-shared-plan-versions.md) is minor with no Breaking
heading and no migration note. Either mark it breaking with a migration note, or keep
role/assignment as @deprecated optional fields for one release and remove them later.

2. build_plan_rebase rejects (or mis-scopes) any request with two or more removals

packages/harness/src/core/build-plan-service.ts:506-535. interim is computed once
before the removal loop, so its diagnostic paths (assignments[N], repositoryIntents[N],
assignments[N].dependencies[M]) index the pre-loop content. Inside the loop, content is
reassigned on every successful removal, but the index is re-derived from the mutated
array:

const assignmentIndex = content.assignments.findIndex(({ id }) => id === resolution.assignmentId);
const relevant = interim.some(({ severity, path }) => severity === "error" && path.startsWith(`assignments[${assignmentIndex}]`));

Failure scenario: a map rebase invalidates assignments at original indices 0 and 2, and the
caller supplies both remove-assignment resolutions. The first removal succeeds; the second
now resolves to index 1, checks interim for an error at assignments[1] (the still-valid
surviving assignment), finds none, and is never added to used. Line 536 then throws
invalid_rebase_resolution even though both resolutions were correct and required — the plan
can never be rebased off that map version. The inverse also happens: after a shift, a
resolution can be accepted on the strength of an unrelated assignment's error.

Match resolutions to diagnostics by entity id, not by array position, or recompute
diagnostics after each removal. build-plan-service.test.ts:267 only exercises a single
removal, so the suite does not catch this.

3. Malformed build_plan_read selectors surface as internal_error / recovery: "retry"

packages/harness/src/server/agent-map-mcp-tools.ts:229 calls parseBuildPlanReadRequest
in the tool handler, outside the service. buildPlanReadToolInputSchema
(core/build-plan-schema.ts:143) intentionally has planId/versionId/semanticDigest
optional, so { kind: "exact" } and { kind: "current", planId: "plan_…" } both pass SDK
validation and reach the handler. The raw ZodError is not a BuildPlanServiceError, so
errorResult falls through to { code: "internal_error", recovery: "retry" } — the calling
agent is told to retry an input that can never succeed, instead of getting the
malformed_input / recovery: "correct" that BuildPlanService.read already produces
(build-plan-service.ts:316). Drop the handler-side parse and let the service own it; the
service re-parses anyway, so this is also a redundant pass. No test covers a malformed
build-plan selector.

4. Plan history quota has no recovery path

BUILD_PLAN_VERSION_HISTORY_LIMIT = 1_024 (shared/build-plan.ts:14) is enforced in
commit (build-plan-service.ts:568) by throwing quota_exceeded, with no trimming,
archival, or compaction anywhere. Receipts get eviction-to-tombstone; plan versions get
nothing. On the 1,025th semantic edit every build_plan_apply and build_plan_rebase for
that project fails permanently, and errorResult maps quota_exceeded to
recovery: "retry" — an infinite retry loop for an agent. docs/shared-build-plan.md
documents the immutable-history contract but never mentions the ceiling or what a caller
should do at it.

Verdict: request changes — findings 1 and 2 must land before merge (a breaking public
type under a minor bump, and a rebase path that is broken for the multi-resolution case
it exists to serve).

Review — PR #806 (round 2, delta since 492bb18)

Round-1 findings 1, 2 and 3 are fixed: the changeset now carries a Breaking heading
with a migration note (minor is the correct breaking bump for 0.14.0), rebase
resolutions are matched by entity id instead of array index and the multi-removal case is
covered by a test, and build_plan_read no longer parses in the handler so malformed
selectors return malformed_input / recovery: "correct" (tested).

1. New — manual_intervention is now returned for a caller-correctable quota error

packages/harness/src/server/agent-map-mcp-tools.ts:97 maps every quota_exceeded to
recovery: "manual_intervention", but that code has three throw sites. One of them,
build-plan-service.ts:197, fires when a single build_plan_apply registers more client
refs than BUILD_PLAN_ID_MAPPING_LIMIT — an oversized request, not an exhausted project.
An agent that submits a large plan is now told an operator must intervene, so it stops,
when splitting the batch would have worked immediately; "correct" is the right signal
there. Give the mapping-limit throw its own code (or an error.details discriminator) and
reserve manual_intervention for the history/receipt ceilings.

2. Round-1 finding 4 only half fixed — the map-version ceiling still retries forever

packages/harness/src/core/agent-map-proposal-service.ts:294 hits the same 1,024
BUILD_PLAN_VERSION_HISTORY_LIMIT and throws
AgentMapWorkspaceStoreError("storage_unavailable"), which
agent-map-mcp-tools.ts:100 maps to recovery: "retry". A project whose map history is
permanently full therefore gets a transient-sounding storage error and an infinite retry
loop — exactly the behaviour the plan-side fix removed — and it contradicts the new
docs/shared-build-plan.md:39-44 paragraph, which states the bound is terminal and
operator-resolved. Surface it as a quota error with the same terminal recovery, and say in
the doc that map versions share the bound.

Note: I could not execute the harness tests in this environment (command approval), so the
fixes above are assessed by reading the diff, not by a green run.

Verdict: approve with comments — both remaining items are error-signalling defects, not
data-integrity ones; neither blocks merge if you'd rather land them as a follow-up.

/review

Review — PR #806 (round 3, delta since e19c11fb)

Both round-2 findings are fixed: the ID-mapping ceiling now throws its own
request_too_large code mapped to recovery: "correct" (tested at the service and MCP
layers), and the map-version ceiling throws AgentMapProposalQuotaError →
quota_exceeded / manual_intervention with a pre-mutation, no-write test and a doc
update. Nothing the earlier rounds said was wrong.

1. New — mapOperationHistory is the one history with no bound at all

packages/harness/src/core/agent-map-proposal-service.ts:303 appends one record per
accepted operation and nothing ever trims it. There is no limit constant for it
(shared/build-plan.ts:14-18 bounds map versions, plan versions, receipts and
tombstones), and agent-map-aggregate-migration.ts:251 validates the length of every
other collection but skips this one. Unlike mapVersions, it also grows on
digest-unchanged proposals, which take the early-return path at line 308.

Failure scenario: a long-lived project accumulates operation records without limit in the
single aggregate JSON that every agent_map_propose reads, structuredClones and
rewrites, while lines 144/150/162/287 scan it linearly per request — latency and file size
degrade with no ceiling and no terminal signal, and the migration validator will accept an
aggregate of any size. This directly contradicts docs/shared-build-plan.md:39-44, which
now tells readers histories are bounded at 1,024 and exhaustion is terminal. Either bound
and classify it like the others, or say in the doc that operation records are unbounded.

2. request_too_large is agent-facing but undocumented

recovery: "correct" tells the agent to fix its request, but neither
docs/shared-build-plan.md nor the README mentions BUILD_PLAN_ID_MAPPING_LIMIT (128
client refs per request) or the new code, and error.details carries only
affectedPaths: ["operations.0.content"] — no limit value. The agent has nothing to
correct against. Add the limit to the doc's bounds paragraph.

Nit: agent-map-workspace-store.ts:318,330 still throw storage_unavailable → retry
for the brief-history and receipt ceilings; unreachable until SAP-3150, but same defect.

Tests were not runnable here (command approval), so this is a read of the diff.

Verdict: approve with comments — finding 1 is a durability/latency gap, not a
correctness break, and finding 2 is a doc line.

Superseded by the maintainer-authorized 15-part Agent Map review stack. This PR is closed without merging; its branch and review history remain preserved.

Replacement increments:

• Part 08/15: feat(harness): define shared plan versions [Agent Map 08/15] #827 — feat(harness): define shared plan versions
• Part 09/15: feat(harness): migrate shared project state [Agent Map 09/15] #828 — feat(harness): migrate shared project state
• Part 10/15: feat(harness): expose shared plan authoring [Agent Map 10/15] #829 — feat(harness): expose shared plan authoring

The complete testing branch remains fix/studio-onboarding-followups. Full stack: #819 through #834. Final hosted-review corrections and Mac journey validation are continuing on those same replacement PRs.